@nathanpeck Hey. I was wondering what you thought of applying some of your L3 service extension ideas to the L2 ECS constructs. I really like your approach and I think we can generalize the ideas enough to make them work at L2. I took a crack at such an interface in this PR.

After #13230, I imagine that we could write an extension that adds AppMesh support to an L2-defined Fargate service.

After #13240, we'll be able to add the main container by service extension similar to your ServiceDescription approach.

Thanks

We considered something similar to this as an alternate to building a higher level extensions pattern. However there are a some significant downsides to this. The main issue is that the level two constructs underneath are intended to have minimal mutability. (See my other comment here: #13230 (comment)) So in order to make this work we'd have to make changes to the L2 constructs which will end up introducing a lot of unintended side effects due to the existing expectations of immutability.

The other challenge that I ran into is that extensions often need a way to be aware of each other so that they can make better decisions about their own configuration. For example with App Mesh in specific, when you add App Mesh to a task it needs to change how the task container dependsOn startup order works. You need to build a container dependency tree to ensure that containers start in the right order, otherwise some containers may attempt to start before the Envoy Proxy is up and running, creating a race condition failure.

I think there is definitely room for a simpler extension interface that solves simple use cases like the example you have of packaging up an autoscaling config and reusing it. I think we will see problems with some of the deeper things like App Mesh though.

We considered something similar to this as an alternate to building a higher level extensions pattern. However there are a some significant downsides to this. The main issue is that the level two constructs underneath are intended to have minimal mutability. (See my other comment here: #13230 (comment)) So in order to make this work we'd have to make changes to the L2 constructs which will end up introducing a lot of unintended side effects due to the existing expectations of immutability.

The other challenge that I ran into is that extensions often need a way to be aware of each other so that they can make better decisions about their own configuration. For example with App Mesh in specific, when you add App Mesh to a task it needs to change how the task container dependsOn startup order works. You need to build a container dependency tree to ensure that containers start in the right order, otherwise some containers may attempt to start before the Envoy Proxy is up and running, creating a race condition failure.

I think there is definitely room for a simpler extension interface that solves simple use cases like the example you have of packaging up an autoscaling config and reusing it. I think we will see problems with some of the deeper things like App Mesh though.

@nathanpeck Sounds reasonable with this idea of limited mutability in mind.

I think there is definitely room for a simpler extension interface that solves simple use cases like the example you have of packaging up an autoscaling config and reusing it.

In terms of a simpler extension interface, what do you have in mind? I don't mind making adjustments or going back to the drawing board so as to at least create the reusable configuration part - that's the part I truly care about. For instance, what about generalizing the extension interface to IService, or IEc2Service and IFargateService, rather than giving an opportunity for mutation by targeting the concrete classes?

Edit: Hrm. Actually, neither ecs.IFargateService nor ecs.IEc2Service have autoScaleTaskCount(), so to allow this kind of configuration of autoscaling by extension, I think we'll need the concrete classes. If we keep the protections of the concrete class interfaces as they are, what problems are inherent in targeting the concrete classes for the extension interfaces per this PR?

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
• Commit ID: d3a17af
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

So after discussing it with the team we are going to close this PR because it would add more logic to the L2 construct, while our design goal is to implement higher level logic at L3 instead. One of our goals is to keep the L2 constructs from getting too bloated, so they don't get unwieldily, and implement this type of extension behavior as an L3 construct, in the aws-containers/ecs-service-extensions namespace instead.